Skip to content

[Sim Theme B] Launch menu & simulator lifecycle#23

Open
jmservera wants to merge 13 commits into
mainfrom
jmservera/sim-theme-b-launch-menu
Open

[Sim Theme B] Launch menu & simulator lifecycle#23
jmservera wants to merge 13 commits into
mainfrom
jmservera/sim-theme-b-launch-menu

Conversation

@jmservera

Copy link
Copy Markdown
Owner

Implements Theme B of the Simulation & Visualization PRD (FR-B1..B8). Lane 1 (foundation) — depends on Theme A (registry + GET /simulators). Adds launch/stop endpoints, concurrent + reload-persistent lifecycle, allowlisted start/stop, configurable autostart. Plan: docs/plans/sim-theme-b-launch-menu.md. Draft.

@jmservera
jmservera force-pushed the jmservera/sim-theme-b-launch-menu branch from d2f0f14 to 44a7113 Compare July 21, 2026 11:18
@jmservera
jmservera changed the base branch from jmservera/gazebo-ros-integration to jmservera/sim-theme-e-ros-integration July 21, 2026 11:18
…ncurrency, reload reconnect, autostart (FR-B1..B8)
@jmservera jmservera mentioned this pull request Jul 21, 2026
@jmservera

Copy link
Copy Markdown
Owner Author

🤖 Automated PR review

Launch menu & lifecycle — POST /simulators/{id}/launch|stop, per-sim autostart reconcile, and the interactive menu with polling + namespaced sim:<id> Golden Layout panels that survive pop-out/reload. Base: #26.

Findings

Severity Location Finding
🟡 Medium services/control/server.js (reconcileAutostart) Runs on every control-plane boot and stops any running autostart:false sim. A control-plane restart (crash / restart: unless-stopped) therefore silently kills a simulator the user launched manually from the menu. Consider a one-shot-at-stack-start guard or a "user-touched" marker so reconcile doesn't revert manual launches.
🟢 Low services/control/server.js reconcileAutostart fires once after a fixed UBEROS_AUTOSTART_DELAY_MS (3s); a slow host that hasn't finished creating containers misses reconciliation. A short bounded retry would be more robust.

✅ Security hardening is exemplary: launch/stop uses a registry-derived allowlist distinct from the restart allowlist, the id is charset-constrained by regex and validated against the registry, and only start/stop-by-container-id are issued — no exec/create/shell. Idempotent 204/304 handling, clean polling teardown in onDestroy.

Recommendation: Approve; consider the reconcile-on-restart behavior before merge.

@jmservera jmservera left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated line-anchored review — see the summary comment for the full findings table.

Comment thread services/control/server.js
Base automatically changed from jmservera/sim-theme-e-ros-integration to main July 24, 2026 08:24
@jmservera
jmservera marked this pull request as ready for review July 24, 2026 08:25
Copilot AI review requested due to automatic review settings July 24, 2026 08:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements Theme B (“Launch menu & simulator lifecycle”) by adding simulator launch/stop operations to the control plane and wiring the frontend Simulators menu to start/stop simulators, open/focus simulator panels, and keep simulator state refreshed while the menu is open.

Changes:

  • Frontend: add launch/stop actions, simulator-state polling while the menu is open, and Golden Layout rebuild support for sim:<id> panels via component state.
  • Control service: add allowlisted simulator start/stop endpoints and a boot-time autostart reconcile.
  • Config/docs: document Theme B plan and add env/compose knobs for installed simulators + autostart selection.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
services/frontend/src/lib/panels.js Adds simulator panel rebuild-from-state factory for sim:<id> Golden Layout components.
services/frontend/src/lib/control.js Adds client helpers for POST /control/simulators/{id}/launch and /stop.
services/frontend/src/App.svelte Implements menu actions (launch/stop), panel open/focus, and polling while Simulators menu is open.
services/control/simulators.js Adds UBEROS_SIMULATORS_AUTOSTART override parsing for per-simulator autostart behavior.
services/control/server.js Adds allowlisted simulator start/stop routes + container resolution and boot-time autostart reconcile logic.
docs/plans/sim-theme-b-launch-menu.md Adds a Theme B plan stub documenting FR-B1..B8 scope.
compose.yaml Adds control-service env vars for installed simulators and autostart override.
.env.example Adds example env vars for installed simulators and autostart override (currently with invalid dotenv syntax).
Comments suppressed due to low confidence (4)

services/control/server.js:257

  • reconcileAutostart() ignores the Docker API status code for the stop request. If Docker returns a non-204/304 status, it will be treated as success and autostart intent may not actually be enforced.
        await dockerRequest('POST', `/containers/${container.Id}/stop`);

services/frontend/src/App.svelte:493

  • Same simBusy race applies to stopSim: overlapping async actions can cause simBusy to be cleared by an earlier call while a later call is still pending. Guard against concurrent actions (or track busy per simulator).
  async function stopSim(id) {
    simBusy = id;

services/frontend/src/App.svelte:844

  • While an action is in flight, other simulator buttons remain enabled (because disabled only matches the busy id). Combined with the simBusy race, this makes it easy to trigger overlapping actions that the UI state can't represent. Disabling all simulator actions while simBusy is non-null avoids this class of issues.
                  <button class="svc-reset" disabled={simBusy === sim.id} on:click={() => stopSim(sim.id)}>

services/frontend/src/App.svelte:848

  • Same as above for the Launch button: leaving other rows enabled while one request is pending allows overlapping actions even though state is tracked with a single simBusy value.
                  <button class="svc-reset" disabled={simBusy === sim.id} on:click={() => launchSim(sim.id)}>

Comment thread .env.example Outdated
Comment thread services/frontend/src/lib/panels.js
Comment thread services/control/server.js Outdated
Comment thread services/frontend/src/App.svelte Outdated
@jmservera

Copy link
Copy Markdown
Owner Author

PR summary update for Sim Theme B (Launch menu and simulator lifecycle)

Resolved review feedback and applied targeted fixes:

  • Fixed dotenv syntax in .env.example for simulator variables (valid KEY=VALUE format).
  • Hardened control-plane autostart reconcile in services/control/server.js:
    • Added explicit Docker status validation for start/stop (204/304 only accepted).
    • Prevented reconcile from re-running on every control-plane restart in the same stack instance.
    • Added bounded retry at boot to handle slow container creation.
  • Fixed simulator panel rebuild fallback in services/frontend/src/lib/panels.js:
    • When state is incomplete, fallback now uses id-namespaced route (/sim/{id}/).
  • Fixed async busy-state race in services/frontend/src/App.svelte:
    • Replaced single shared busy flag with per-simulator busy tracking.

Validation:

  • Targeted acceptance suites passed after fixes:
    • s1/s7/s9: pass
    • s7/s9: pass
  • Edited files report no diagnostics errors.

All previously unresolved review threads on this PR have been resolved.

…, update BRD and ADR documentation, improve simulator lifecycle management, and add acceptance tests for Theme B functionality.
Copilot AI review requested due to automatic review settings July 24, 2026 09:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Comment thread services/control/server.js
Comment thread services/control/server.js
Comment thread tests/acceptance/s10-theme-b-lifecycle.spec.js
… health checks

- Updated Theme A and C documentation with implementation details and status summaries.
- Added acceptance tests for Theme C turtlesim functionality and Theme A framework lifecycle.
- Improved stack health check to ensure all services report healthy status after launching.
- Updated Theme F transport tests to validate gzweb routing and retirement of Gazebo noVNC.
Copilot AI review requested due to automatic review settings July 24, 2026 12:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (3)

services/control/server.js:248

  • shouldRunAutostartReconcile() writes the autostart stamp file before reconciliation has actually succeeded. If Docker is unreachable or simulator containers aren’t created yet, reconcileAutostart() returns false but the stamp has already been written, so subsequent retry attempts will be skipped and autostart intent may never be enforced for this stack instance.
async function shouldRunAutostartReconcile() {
  const stamp = STACK_INSTANCE || PROJECT;
  try {
    const prior = (await readFile(AUTOSTART_STAMP_FILE, 'utf8')).trim();
    if (prior === stamp) return false;

services/control/server.js:270

  • reconcileAutostart() returns undefined when shouldRunAutostartReconcile() is false (already reconciled). scheduleAutostartReconcile() treats that as “not done” and will keep retrying pointlessly. Returning a boolean here avoids unnecessary retries and keeps the function’s return contract consistent.
async function reconcileAutostart() {
  if (!(await shouldRunAutostartReconcile())) return;
  let containers;

tests/acceptance/s13-theme-f-transport.spec.js:47

  • The total timing assertion (elapsedMs < 30_000) can fail even when both individual waits succeed, because the test allows up to 30s for #stream-status and then up to another 30s for #state (sequential waits). If the DOM updates are staggered, elapsedMs can exceed 30s and make the test flaky.
    const started = Date.now();
    await page.goto('/gzweb/');
    await expect(page.locator('#stream-status')).toHaveText(/connected/i, { timeout: 30_000 });
    await expect(page.locator('#state')).toHaveText(/connected/i, { timeout: 30_000 });

    const elapsedMs = Date.now() - started;
    // Informative ceiling to keep deterministic CI signal while documenting
    // FR-F5 behavior in variable host environments.
    expect(elapsedMs).toBeLessThan(30_000);

Comment thread services/control/server.js Outdated
Comment thread tests/acceptance/s11-theme-c-turtlesim.spec.js
Comment thread tests/acceptance/s13-theme-f-transport.spec.js
Comment thread docs/plans/sim-theme-b-launch-menu.md
Copilot AI review requested due to automatic review settings July 24, 2026 13:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Comment thread services/control/simulators.js Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 24, 2026 14:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.

Comment on lines +305 to +307
const retries = Math.max(1, Number(process.env.UBEROS_AUTOSTART_RETRIES || 5));
const retryMs = Math.max(250, Number(process.env.UBEROS_AUTOSTART_RETRY_MS || 1500));
let attempts = 0;
Comment on lines +472 to +474
setTimeout(() => {
scheduleAutostartReconcile();
}, Number(process.env.UBEROS_AUTOSTART_DELAY_MS || 3000));
Comment on lines +552 to +556
function startSimPolling() {
refreshSimulators();
stopSimPolling();
simPollTimer = setInterval(refreshSimulators, 2500);
}
Comment on lines +34 to +35
* FR-B6 evidence: reload reconnect and running-state persistence validated by `tests/acceptance/s10-theme-b-lifecycle.spec.js`.
* FR-B8 evidence: autostart override matrix validated by `tests/acceptance/s10-theme-b-lifecycle.spec.js` plus default env wiring in `.env.example` and `compose.yaml`.
Comment on lines +1 to +3
// S10 - Theme B lifecycle acceptance coverage for FR-B6 and FR-B8.
// Verifies reload reconnect and autostart override semantics via control-plane
// simulator state without requiring direct Docker API assertions in test code.
Comment on lines +50 to +55
const started = Date.now();
await page.goto('/gzweb/');
await expect(page.locator('#stream-status')).toHaveText(/connected/i, { timeout: 30_000 });
await expect(page.locator('#state')).toHaveText(/connected/i, { timeout: 30_000 });

const elapsedMs = Date.now() - started;
Comment on lines +34 to +42
const retiredNoVnc = await request.get('/sim/gazebo/novnc/', { maxRedirects: 0 });
expect([200, 404, 502]).toContain(retiredNoVnc.status());
if (retiredNoVnc.status() === 200) {
const fallbackHtml = await retiredNoVnc.text();
// Some proxy/frontend variants route unknown paths to the SPA shell.
// Accept that only when the body is clearly not a noVNC endpoint.
expect(fallbackHtml).not.toContain('noVNC');
expect(fallbackHtml).toContain('<!doctype html');
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants